-
Notifications
You must be signed in to change notification settings - Fork 5.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add syncing models utility to ivy #28818
Conversation
My thinking was just exposing these as |
@YushaArif99 Couldn't we just store all helper functions like this in a new file like Also, I'd suggest we could expose a general |
Sure! This is indeed a better UX. I was only inclined to not go this route as the logic contained within these utility helpers is framework-specific. And so it seems to deviate from the general convention of Otherwise, this is definitely cleaner. Based on this, should we still go forward with having these inside |
Yeah that's a good idea 👍🏼 |
@YushaArif99 I don't see an issue with it, because you'd have to import torch into the tensorflow backend anyway, which is also against the general convention of ivy. But I guess if we do expose |
Yeah I think exposing I'll go ahead with this approach then. Thanks for the suggestions both! |
Hey @hmahmood24 @Sam-Armstrong, could you guys give this a final look and confirm whether we're all okay with this approach. I have added all the syncing logic inside |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! quick question though, could we generalise this to just be called ivy.sync_models
, and in the longer term use this same function for syncing models from any framework? I feel like that would be a cleaner api. for now we can just throw a meaningful exception if a torch.nn.Module is not the first argument. what do you think @YushaArif99?
yeah we can definately do that. We can add |
Co-authored-by: Sam Armstrong <88863522+Sam-Armstrong@users.noreply.github.com>
…odels` and adding a `source` kwarg to route to the appropriate helper.
@YushaArif99 do we need source/target kwargs, can't we just infer based on the input types? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@YushaArif99 LGTM. Agreed with @Sam-Armstrong's point though that keeping a single ivy.sync_models
API should be cleaner if we can implement that 👍🏼
@Sam-Armstrong that would require us importing all frameworks inside |
@YushaArif99 maybe we could check if each framework is in
|
@YushaArif99 Can't we just use this which shouldn't require us importing any fws |
…tances of native modules by traversing through the `mro` chain.
…and instead using `_is_submodule`
Wanted to have your thoughts on how to expose these utility functions? I think it makes sense to have them inside the
ivy.functional.backends.tensorflow.module.py
module. But this creates an issue as to how we import this helper. So there wont be anyivy.sync_models_torch_and_tf
but instead we'll have to do:What do you guys suggest we do here @Sam-Armstrong @hmahmood24